Z-ordering for ground geometry entities #6362

hpinkos · 2018-03-22T19:08:58Z

GeometryVisualizer creates different batches based on the value of the zIndex property
Works for Corridor, Ellipse, Polygon and Rectangle
Only works for static entities
Only works when both height and extrudedHeight are undefined

TODO

CHANGES.md
Specs

cesium-concierge · 2018-03-22T19:08:59Z

@hpinkos, thanks for the pull request! Maintainers, we have a signed CLA from @hpinkos, so you can review this at any time.

⚠️ I noticed that CHANGES.md has not been updated. If this change updates the public API in any way, fixes a bug, or makes any non-trivial update, please add a bullet point to CHANGES.md and comment on this pull request so we know it was updated. For more info, see the Pull Request Guidelines.

I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome.

🌍 🌎 🌏

mramato · 2018-03-22T19:22:17Z

Apps/Sandcastle/gallery/Ordering Geometry.html

@@ -0,0 +1,93 @@
+<!DOCTYPE html>


I would rename this to Z-Indexing Geometry, to both be consistent with our internal terminology and because Z-Indexing is the standard term.

mramato · 2018-03-22T19:28:07Z

Thanks @hpinkos!, at initial glance, this approach all looks good to me. Just bump when you think it's ready for a full review.

We should hook this up to KML, I'm pretty sure it will be trivial. If it looks more complicated we can do it in a separate PR after this.

mramato · 2018-03-22T19:29:03Z

We'll also want to make sure we update czml-writer once this is ready and add CZML support (in a separate PR).

hpinkos · 2018-03-22T21:38:20Z

Source/DataSources/GeometryVisualizer.js

+                var zIndex = updater.zIndex;
+                var batchArray = this._groundColorBatches[zIndex];
+                if (!defined(batchArray)) {
+                    batchArray = createGroundColorBatch(this._orderedGroundPrimitives.add(new PrimitiveCollection()));


Right now I create a three new StaticGroundGeometryColorBatch (one for each classification type) and one new PrimitiveCollection() for each zIndex.
And I never remove them (the PrimitiveCollections get removed and destroyed on GeometryVisualizer.destroy)

Do you think this is a problem? I didn't think it would be because the zIndex has to be ConstantProperty so we shouldn't end up with empty primitive collections unless someone changes the zIndex value.

Instead of creating a StaticGroundGeometryColorBatch for each classification type, do you think I should create them only if that classification type gets used?

Just because it's a constant property doesn't mean it can't change, it just means it won't change with respect to simulation time. I think we should try and delay creating anything until we actually need it, perhaps the key to _groundColorBatches can be the zIndex and type together?

Is there a reason we also can't remove it if/when it's empty?

Another thing about this feature that I don't think was made clear is that zIndex only works for completely non-time-varying ground primitives. It has nothing to do with zIndex being constant or not. The way you have the code written here, if color/positions/etc.. is time-dynamic, z-indexing doesn't work. (Unless I'm missing something?)

I think we should try and delay creating anything until we actually need it, perhaps the key to _groundColorBatches can be the zIndex and type together?

Good idea, I think that approach will work

Is there a reason we also can't remove it if/when it's empty?

I'm not sure exactly how to figure out whether a batch is empty. StaticGroundGeometryColorBatch doesn't have a length property or anything.

Another thing about this feature that I don't think was made clear is that zIndex only works for completely non-time-varying ground primitives.

Yes that's true. I have no idea how to maintain zIndex across the StaticGroundGeometryColorBatch and the DynamicGeometryBatch without making bigger changes. There would have to be some class that aggregates both and manages the primitives.

I have no idea how to maintain zIndex across the StaticGroundGeometryColorBatch and the DynamicGeometryBatch without making bigger changes.

Actually, this is probably something we'll need anyway for when the ground primitive material changes come in. I'll have to think about this. I have an idea for what I want to do.

OK, it might be worth holding off this PR until after ground primitive materials are ready then. I think this initial attempt was definitely worth it; but to avoid churn it might be better to do everything at once.

hpinkos · 2018-03-22T21:38:36Z

@mramato this is ready for a full review

mramato · 2018-05-09T15:25:38Z

@mramato this is ready for a full review

I thought we were waiting for ground primitives with materials before merging this because we were worrying about it's impact on this code. Is that not the case?

mramato · 2018-05-09T15:26:08Z

Nevermind, I just noticed your comment was from March, GitHub notified me that this thread was updated but it was just your merge of master, sorry for the noise.

hpinkos · 2018-05-09T15:27:37Z

No problem =) Yeah, I'm working on merging in Gary's ground material stuff now, and this should be ready for a review when I'm finished with that

hpinkos · 2018-05-09T19:19:27Z

The new changes I made will make this trivial to integrate into Gary's branch. I think this should be merged first since Gary's having some texture woes, this change is pretty much ready to go. I just have to write tests.

mramato · 2018-05-09T19:29:33Z

Source/Core/oneTimeWarning.js

@@ -47,5 +47,7 @@ define([

    oneTimeWarning.geometryOutlines = 'Entity geometry outlines are unsupported on terrain. Outlines will be disabled. To enable outlines, disable geometry terrain clamping by explicitly setting height to 0.';

+    oneTimeWarning.geometryZIndex = 'Entity geometry with zIndex are unsupported when the entity is dynamic, or height or extrudedHeight are defined.  zIndex will be ignored';


Are dynamic entities still not supported? This message appears to only be used in one place and doesn't check anything about dynamic.

Oh yeah no, I fixed that. I'll update the message

mramato · 2018-05-09T19:31:42Z

Source/DataSources/CorridorGraphics.js

+         * Gets or sets the zIndex Property specifying the ordering of the corridor.  Only has an effect if the coridor is static and neither height or exturdedHeight are specified.
+         * @memberof CorridorGraphics.prototype
+         * @type {ConstantProperty}
+         * @default 0


All of the defaults say 0, but there's not actually a default is there? It's just undefined.

undefined === 0 for zIndex. I defaultValue(zIndex, 0) in the geometry updaters

mramato · 2018-05-09T19:41:10Z

Source/DataSources/KmlDataSource.js


        var ellipsoid = dataSource._ellipsoid;
        var coordinates = readCoordinates(coordinatesNode, ellipsoid);
        var polyline = styleEntity.polyline;
        if (canExtrude && extrude) {
+            if (defined(zIndex)) {


This check isn't needed, Google Earth silently ignores zIndex for extruded geometry and since we match GE/KML, there's no reason for us to warn here either.

mramato · 2018-05-09T19:41:44Z

Source/DataSources/KmlDataSource.js

        } else {
+            if (defined(zIndex)) {
+                oneTimeWarning('kml-gx:drawOrder', 'KML - gx:drawOrder is not supported in LineStrings');


Maybe extend this message to say "when clampToGround is false".

mramato · 2018-05-09T19:51:17Z

Source/Scene/PrimitiveCollection.js

@@ -42,6 +42,9 @@ define([
        this._primitives = [];
        this._guid = createGuid();

+        // Used by the OrderedGroundPrimitiveCollection
+        this._zIndex = undefined;


Rather than glob a specific zIndex property onto here, and since PrimitiveCollection is a primitive, and primitives normally have ids, why no add an id property as the zIndex. This way there's no reason to have a special case here and the PrimitiveCollection is owned by the OrderedGroundPrimitiveCollection anyway, so it's not weird to use id as the z-index. (we do similar things elsewhere.)

Good idea, thanks!

Nevermind, we spoke offline and decided to leave it as is

mramato · 2018-05-09T20:03:12Z

Source/Scene/OrderedGroundPrimitiveCollection.js

+     * @private
+     */
+    function OrderedGroundPrimitiveCollection() {
+        this._length = 0;


I think you can shorten/simplify this class by using AssociativeArray, you've basically rolled your own here.

Nope, AssociativeArray has no concept of order and that's critical for this to work

Ahh, you're absolutely correct. Sorry.

mramato · 2018-05-09T20:08:03Z

Looks good, bump again when tests are ready and I'll do a final smoke screen and merge. Thanks!

hpinkos · 2018-05-09T20:45:12Z

@mramato ready for a full review!

mramato · 2018-05-10T16:14:21Z

I also didn't think to do any type checking.

You should probably check that it's a Number and throw a developer error if not. This can be done at the same time you call getValue. I believe we normally defer to the primitive layer for these kind of checks, but since this is all Entity API, then we need to do it ourselves. Please also add specs to cover these new checks.

hpinkos · 2018-05-10T16:15:11Z

I was going to add a check to OrderedGroundPrimitiveCollection

mramato · 2018-05-10T16:18:08Z

I was going to add a check to OrderedGroundPrimitiveCollection

That's fine, as long as it provides a useful call stack so that the Developer can easily find out which object is the problem. It the callstack isn't useful, then it probably needs to happen sooner.

hpinkos · 2018-05-10T16:38:03Z

@mramato anything else?

hpinkos · 2018-05-21T20:14:05Z

@mramato this is ready for another look.

There's some weirdness that was introduced with materials on ground primitives. Ground primitives with different classification types cannot be ordered because they're rendered in separate passes. Because materials on ground primitives is only supported for ClassificationType.TERRAIN, the classification type for any ground geometry entity with a material defaults to ClassificationType.TERRAIN. However, all ground geometry with a color defaults to ClassificationType.BOTH. And therefore, z index doesn't work when intermingling colored ground geometry and textured ground geometry unless you specify ClassificationType.TERRAIN for all the colored ground geometry.

mramato · 2018-05-22T14:38:38Z

There's some weirdness that was introduced with materials on ground primitives. Ground primitives with different classification types cannot be ordered because they're rendered in separate passes. Because materials on ground primitives is only supported for ClassificationType.TERRAIN, the classification type for any ground geometry entity with a material defaults to ClassificationType.TERRAIN. However, all ground geometry with a color defaults to ClassificationType.BOTH. And therefore, z index doesn't work when intermingling colored ground geometry and textured ground geometry unless you specify ClassificationType.TERRAIN for all the colored ground geometry.

In the interest of avoiding user confusion, can we just make everything default to ClassificationType.TERRAIN and then users have to opt-in to BOTH?

Also, do we have a separate github issue written up for this? It's a bug even without z-indexing because you can still order ground primitives, right?

mramato · 2018-05-22T15:35:55Z

Probably a bug in my gallery generation code, but any idea why Z-Indexing Geometry is showing up twice in the New in 1.45 label? Doesn't have to hold up this PR.

mramato · 2018-05-22T15:37:03Z

Update the screenshot on the new example as well.

mramato · 2018-05-22T15:38:32Z

CHANGES.md

@@ -30,6 +31,7 @@ Change Log

 ##### Breaking Changes :mega:
 * Removed `Scene.copyGlobeDepth`. Globe depth will now be copied by default when supported. [#6393](https://github.com/AnalyticalGraphicsInc/cesium/pull/6393)
+* The default `classificationType` for `GroundPrimitive`, `CorridorGraphics`, `EllipseGraphics`, `PolygonGraphics` and `RectangleGraphics` is now `ClassificationType.TERRAIN`.  If you wish the geometry to color both terrain and 3D tiles, pass in the option `classificationType: Cesium.ClassificationType.BOTH`.


Probably add a mention here about BOTH not being supported for ground geometry without materials?

It's mentioned elsewhere

mramato · 2018-05-22T16:13:52Z

That's all I have, let me know when this is ready for merge.

hpinkos · 2018-05-22T16:14:11Z

@mramato this is ready now

mramato · 2018-05-22T16:14:31Z

Did this during test, it was trippy enough to share.

hpinkos · 2018-05-22T16:14:52Z

Probably a bug in my gallery generation code, but any idea why Z-Indexing Geometry is showing up twice in the New in 1.45 label?

Yeah, I think that's a bug in the gallery generation code. I'll write up an issue

hpinkos · 2018-05-22T16:51:48Z

@mramato bump

mramato · 2018-05-22T17:33:08Z

Was just waiting for it to go green.

eatjhuapl · 2018-06-14T20:51:08Z

Does zIndex work with geojson datasource?

hpinkos · 2018-06-18T14:26:29Z

@eatapl we haven't hooked this up to GeoJson because I couldn't find anything in the GeoJSON spec that can be used to specify polygon ordering. Does GeoJSON have a property used for ordering that I missed? Thanks!

edit: @eatapl please reply in #4108

mramato · 2018-06-18T14:37:50Z

@eatapl Just to add-on to @hpinkos' comment. The GeoJSON spec does not cover Z ordering but you can definitely assign zIndex values to entities created by GeoJSON, just like you can add styling and user any other Entity API features.

hpinkos added 2 commits March 22, 2018 14:44

working z-indexing [ci skip]

bd55144

sandcastle example

c970417

mramato reviewed Mar 22, 2018

View reviewed changes

hpinkos added 2 commits March 22, 2018 15:57

Fix ordering, add to kml

ce145b5

specs and CHANGES.md

e34d7c5

hpinkos force-pushed the z-index branch from 2535fdd to e34d7c5 Compare March 22, 2018 21:26

hpinkos commented Mar 22, 2018

View reviewed changes

Merge branch 'master' into z-index

343f3cf

hpinkos mentioned this pull request Apr 9, 2018

Height Reference for Corridor, Ellipse, Polygon and Rectangle #6434

Closed

10 tasks

Merge branch 'master' into z-index

a4da7e5

zIndex for static and dynamic geometry

85df9ef

mramato reviewed May 9, 2018

View reviewed changes

specs

423762a

hpinkos added 2 commits May 10, 2018 11:26

Merge branch 'master' into z-index

a1636d5

fix specs

e04ab01

type check, negative numbers

3f408e4

hpinkos added 3 commits May 21, 2018 13:19

Merge branch 'master' into z-index

045a2ea

fix merge

ba34dc8

Merge branch 'master' into z-index

b0a31b7

Merge branch 'master' into z-index

8d7902c

mramato reviewed May 22, 2018

View reviewed changes

hpinkos force-pushed the z-index branch from 840fd3e to 1c13570 Compare May 22, 2018 16:09

fix classification type

e656a8d

hpinkos force-pushed the z-index branch from 1c13570 to e656a8d Compare May 22, 2018 16:12

fix CHANGES.md

30ae474

mramato merged commit 87cc8b0 into master May 22, 2018

mramato deleted the z-index branch May 22, 2018 17:33

hpinkos mentioned this pull request Jun 21, 2018

Height Reference for Corridor, Ellipse, Polygon and Rectangle #6717

Merged

4 tasks

		@@ -47,5 +47,7 @@ define([

		oneTimeWarning.geometryOutlines = 'Entity geometry outlines are unsupported on terrain. Outlines will be disabled. To enable outlines, disable geometry terrain clamping by explicitly setting height to 0.';

		oneTimeWarning.geometryZIndex = 'Entity geometry with zIndex are unsupported when the entity is dynamic, or height or extrudedHeight are defined. zIndex will be ignored';

Z-ordering for ground geometry entities #6362

Z-ordering for ground geometry entities #6362

Conversation

hpinkos commented Mar 22, 2018 • edited Loading

cesium-concierge commented Mar 22, 2018

Choose a reason for hiding this comment

mramato commented Mar 22, 2018

mramato commented Mar 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hpinkos commented Mar 22, 2018

mramato commented May 9, 2018

mramato commented May 9, 2018

hpinkos commented May 9, 2018

hpinkos commented May 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mramato commented May 9, 2018

hpinkos commented May 9, 2018

mramato commented May 10, 2018

hpinkos commented May 10, 2018

mramato commented May 10, 2018

hpinkos commented May 10, 2018

hpinkos commented May 21, 2018

mramato commented May 22, 2018

mramato commented May 22, 2018

mramato commented May 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mramato commented May 22, 2018

hpinkos commented May 22, 2018

mramato commented May 22, 2018

hpinkos commented May 22, 2018

hpinkos commented May 22, 2018

mramato commented May 22, 2018

eatjhuapl commented Jun 14, 2018

hpinkos commented Jun 18, 2018 • edited Loading

mramato commented Jun 18, 2018

hpinkos commented Mar 22, 2018 •

edited

Loading

hpinkos commented Jun 18, 2018 •

edited

Loading